@claude review

Claude finished @cliffhall's task in 2m 40s —— View job

Claude Code Review

• Read the four state manager source files
• Compare against the existing managedRequestorTasksState precedent (feat(servers): persist server list to ~/.mcp-inspector/mcp.json (#1343) #1349)
• Read the four state manager test files and peer hook tests
• Check resourceSubscriptionsState update
• Post review findings

Verdict: LGTM ✅

This is a clean, well-scoped follow-up to #1349. The fix faithfully mirrors the precedent set there, the comments call out exactly why the gate exists (with a -32601 explanation in each), and the test coverage is symmetric across the four managers. Approving in spirit — no changes requested.

What I checked

Implementation parity with #1349. The new gate in each of the four managers is identical in shape to core/mcp/state/managedRequestorTasksState.ts:116:

if (!client.getCapabilities()?.<cap>) {
  this.<list> = [];
  this.dispatchTypedEvent("<list>Change", this.<list>);
  return this.get<List>();
}

Placement is right after the connected guard (correct — there's no point checking capabilities on a disconnected client), and it's reached by both the public refresh() and the onConnect listener that calls void this.refresh() at managedToolsState.ts:32-34 (and the three peers).

Capability mapping for resource templates. Gating ManagedResourceTemplatesState on capabilities.resources (not a nonexistent resourceTemplates) is the right call — the MCP spec defines no separate templates capability, and resources/templates/list is part of the resources surface. The comment at managedResourceTemplatesState.ts:79-85 calls this out explicitly, which I appreciate — easy to land a subtle bug there otherwise.

Connect-path timing. Confirmed at core/mcp/inspectorClient.ts:1929 that this.capabilities = this.client.getServerCapabilities() is assigned before the connect event dispatches, so the gate sees populated capabilities even on the connect-triggered refresh. Good.

Test surface. I also grepped the remaining FakeInspectorClient consumers (pagedToolsState, pagedPromptsState, pagedResourcesState, pagedResourceTemplatesState, useResourceSubscriptions, log states, etc.) — none of them drive a Managed*State.refresh() path that would now hit the gate, so the test updates in this PR appear complete.

Minor observations (non-blocking)

1. Always dispatches *Change on a capability-absent refresh. Even when the list is already empty (the common case on initial connect to a capability-less server), the gate clears + dispatches unconditionally. For a tools-less server, every connect now fires one extra toolsChange([]) event. Harmless in practice (downstream selectors will no-op on referential equality / empty checks), and it matches the precedent — just flagging it.

2. No capabilitiesChange listener. If a server's advertised capabilities ever changed mid-session (e.g., via FakeInspectorClient.setCapabilities in a test or some future real-client scenario), the managers wouldn't re-attempt the list. Also matches the precedent and is fine — real MCP servers don't renegotiate capabilities, and the fake's setter is only used to drive capabilitiesChange event tests.

3. Microtask-flush pattern in the new "connect" tests. The await Promise.resolve(); await Promise.resolve(); idiom in e.g. managedToolsState.test.ts:69-70 is mirrored from feat(servers): persist server list to ~/.mcp-inspector/mcp.json (#1343) #1349 and works for the current shape of refresh(). If refresh ever grows a third async hop before the listX call, these tests could pass for the wrong reason (race rather than gate). A vi.waitFor(() => expect(listX).not.toHaveBeenCalled()) would be more robust, but consistency with the precedent has its own value here.

Nits

• Several comments repeat the same -32601 explanation across the four files. Not worth deduplicating into a helper — the per-file context is useful when reading any one of them in isolation.
• The PR body's explanation of why "tools-only / resources-only servers" don't surface today but might in the future is exactly the kind of motivation that's nice to see for a "no observable behavior change in practice" fix.
  · feat/1350-gate-list-on-capability

Thanks for the thorough review! No code changes — all three observations are deliberate choices to track the #1349 precedent, but here's the reasoning per point:

1. Unconditional *Change([]) dispatch on a capability-absent refresh. Kept as-is for parity with ManagedRequestorTasksState. The extra empty-list event on connect to a capability-less server is harmless (downstream useManagedX selectors land on an already-empty list), and keeping the four managers byte-for-byte identical to the tasks precedent is worth more than shaving one no-op event. If we ever want to suppress it, it should be done across all five managers at once, not just the four here.

2. No capabilitiesChange listener. Intentional and matches precedent — real MCP servers negotiate capabilities once at initialize and don't renegotiate mid-session, so there's no live scenario where a manager would need to re-attempt a list after the gate closed. The fake's setCapabilities only exists to drive capabilitiesChange event tests, not a real flow.

3. Microtask-flush idiom in the new connect tests. Fair point that await Promise.resolve() ×2 asserts the gate only for the current shape of refresh(). I kept it identical to feat(servers): persist server list to ~/.mcp-inspector/mcp.json (#1343) #1349's tests for consistency, but I agree vi.waitFor would be more robust against a future third async hop. Rather than diverge one PR's tests from the precedent, I'll track hardening all five managers' connect tests (swap the double-flush for vi.waitFor) as a small follow-up so they stay uniform — will link the issue here.

On the nits: agreed on both — the per-file -32601 comments are worth keeping un-deduplicated for in-isolation readability, and glad the PR-body motivation was useful.

Filed the test-hardening follow-up as #1396 (swap the double microtask flush for vi.waitFor across all five Managed*State connect-path tests, kept uniform with the #1349 original).